Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

make to_vec(::Integer) an empty vector #189

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

make to_vec(::Integer) an empty vector #189

wants to merge 6 commits into from

Conversation

mzgubic
Copy link
Member

@mzgubic mzgubic commented Aug 4, 2021

Closes #188

breaks a few rules in ChainRules, need to take a look at that before merging

mzgubic
mzgubic commented Aug 5, 2021
View reviewed changes
Project.toml
@@ -1,6 +1,6 @@
name = "FiniteDifferences"
uuid = "26cc04aa-876d-5657-8c51-4c34ba976000"
version = "0.12.18"
version = "0.12.19"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be breaking instead? i think of it as a bug fix

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My view is treat it as a bug fix for now -- if it turns out to be super breaking and we later decide it's actually a breaking change, we can always bump the minor version number then.

mzgubic
mzgubic commented Aug 5, 2021
View reviewed changes
test/to_vec.jl
@@ -67,8 +67,11 @@ function test_to_vec(x::T; check_inferred=true) where {T}
return nothing
end

myrandn(T::Type{<:Number}, args...) = randn(T, args...)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i should probably rename this to something sensible.

robust_randn, randn_ints_ok, randn_that_doesnt_hate_ints

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this should have randn in the name, since the integer method doesn't really sample from a normal distribution. Perhaps something a bit more verbose like rand_number?

This was referenced Aug 5, 2021
Merged
Closed
willtebbutt
willtebbutt reviewed Aug 6, 2021
View reviewed changes
src/grad.jl
# v == []
# b(v) == 1
# which means that jvp always returns the integer itself, since [] - [] == []
_int2zero(x) = x
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect that this will miss a few cases (e.g. vectors of integers), but probably that's fine for now -- we can revisit later it needs be.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a more general note: is the need for this function a manifestation of the fact that FiniteDifferences doesn't really know how to handle tangents properly?

@mcabbott mcabbott closed this Sep 5, 2021
@mcabbott mcabbott reopened this Sep 5, 2021
@willtebbutt
Copy link
Member

#191 closed in favour of this PR.

@mzgubic what's the status of this? Any chance we could push it through?

@mzgubic
Copy link
Member Author

mzgubic commented Sep 27, 2021 •
edited
Loading

I tackled this before leaving for holidays but didn't have time to continue when I came back. I do remember it was kind of annoying as some weird things were popping up (one symptom is the _int2zero bandaid)

(feel free to take over)

@willtebbutt
Copy link
Member

I think the underlying problem is related to Example 2 from this CRTU issue. Need to confirm by working through the things that are breaking though.

@willtebbutt
Copy link
Member

I think the underlying problem is related to Example 2 from this CRTU issue. Need to confirm by working through the things that are breaking though.

I did a bit more digging into what it would take to implement this. It turns out that we're also missing a way to say "treat t as a tangent for x and convert it to a vector of reals". This functionality is needed in j′vp The distinction is most plain for integers:

to_vec(5) should clearly return an empty vector because integers are non-differentiable, however, if we have a function tangent_to_vec(primal, tangent) where tangent is a tangent for primal, tangent_to_vec(1.0, 5) should return a length 1 vector containing 5.

Similarly, while to_vec(primal::Diagonal) should return a length size(primal, 1) vector (if we're treating things structurally), tangent_to_vec(primal::Matrix{Float64}, tangent::Diagonal{Float64}) should return a length length(primal) vector.

Very doable, just requires some work.

@mzgubic mzgubic mentioned this pull request Sep 28, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willtebbutt willtebbutt willtebbutt left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Should to_vec(::Integer) return an empty vector
3 participants
@mzgubic @willtebbutt @mcabbott